Skip to content

fix: resolve XState refactor review findings#1

Open
kaigler wants to merge 17 commits intodevelopfrom
pr-1524-fresh
Open

fix: resolve XState refactor review findings#1
kaigler wants to merge 17 commits intodevelopfrom
pr-1524-fresh

Conversation

@kaigler
Copy link
Owner

@kaigler kaigler commented Jan 27, 2026

Summary

  • Fix TASK_STOP regression - add fallback when XState disabled
  • Fix dual status emission in exit handler and execution-progress handler
  • Cache isXstateEnabled() with 5-second TTL for performance
  • Initialize XState actor with correct state based on persisted task status
  • Add fallback for worktree merge/discard notifications
  • Fix TASK_REVIEW approval XState desync
  • Extract shared emitStatusChange logic to task-state-utils.ts
  • Rename awaitingPlanReview to awaiting_plan_review for consistency

Test plan

  • All frontend tests pass (2504 passed)
  • All backend tests pass (2011 passed)
  • Lint passes
  • Manual test: Stop task with XState enabled/disabled
  • Manual test: Process exit transitions
  • Manual test: Task approval flow

🤖 Generated with Claude Code

kaigler and others added 15 commits January 25, 2026 01:17
- Add cleanup methods to TaskStateManager to prevent memory leaks
- Fix processExitedPlanReview guard to check exitCode === 0
- Add fallback transition for PROCESS_EXITED with 'stopped' reviewReason
- Add i18n translations for 'stopped' review reason (en/fr)
- Fix TypeScript errors in agent-events-handlers.ts and task-store.ts
- Fix XState v5 compatibility (remove snapshot.changed reference)
- Add proper type annotations for XState machine

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Merge conflict resolutions:
- claude-profile-manager.ts: Combined imports from both branches
- execution-handlers.ts: Combined detectWorktreeBranch with TaskStateMachine imports
- worktree-handlers.ts: Combined stripAnsiCodes with TaskStateMachine imports

PR Review Fixes (5 HIGH/MEDIUM issues):

1. [HIGH] XState guard order for completed tasks
   - Added !(event.hasSubtasks && event.allSubtasksDone) check to
     processExitedPlanReview guard to prevent completed tasks from
     going to awaitingPlanReview instead of human_review

2. [HIGH] cleanupTaskStateManager() never called on app shutdown
   - Added import and call in before-quit handler in index.ts
   - XState actors are now properly stopped on app exit

3. [HIGH] cleanupTask() never called - actors accumulate
   - Added cleanupTask() call in TASK_DELETE handler
   - Updated registerTaskCRUDHandlers to accept getMainWindow parameter

4. [HIGH] isXstateEnabled() feature flag unused
   - Added conditional checks around all XState manager calls
   - XState is now properly gated by the feature flag setting

5. [MEDIUM] Unused taskStateMachine variable in worktree-handlers
   - Removed unused variable and import from worktree-handlers.ts

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The package-lock.json was missing the xstate dependency that's required
by apps/frontend/package.json. This caused npm ci to fail in CI.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix Python formatting (ruff) in cli/main.py and spec_runner.py
- Update test assertions to include 4th reviewReason parameter
  for task:statusChange events

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks for your first PR!

A maintainer will review it soon. Please make sure:

  • Your branch is synced with develop
  • CI checks pass
  • You've followed our contribution guide

Welcome to the Auto Claude community!

@github-actions github-actions bot added bug Something isn't working area/fullstack size/XL labels Jan 27, 2026
kaigler and others added 2 commits January 27, 2026 14:18
Addresses all findings from PR AndyMik90#1524 code review:

HIGH priority:
- Fix TASK_STOP regression: add fallback when XState disabled
- Fix dual status emission in exit handler: make XState/fallback mutually exclusive
- Fix dual status emission in execution-progress handler: same pattern

MEDIUM priority:
- Cache isXstateEnabled() result with 5s TTL to avoid disk reads on hot paths
- Initialize XState actor with correct state based on persisted task status
- Add fallback for worktree merge/discard status notifications
- Fix TASK_REVIEW approval XState desync: use handleManualStatus when enabled
- Extract shared emitTaskStatusChange utility to avoid code duplication

LOW priority:
- Fix state naming: rename awaitingPlanReview to awaiting_plan_review
- Fix indentation in worktree-handlers (included in fallback fix)

Test updates:
- Add mock for isXstateEnabled in ipc-handlers.test.ts to test fallback path

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
kaigler pushed a commit that referenced this pull request Jan 27, 2026
…ndyMik90#1456)

* auto-claude: subtask-1-1 - Add GITHUB_AUTH_CHANGED IPC channel constant

* auto-claude: subtask-1-2 - Add async getGitHubTokenForSubprocess() helper to utils.ts

Add a new exported async function getGitHubTokenForSubprocess() that calls
getTokenFromGhCli() to retrieve fresh GitHub tokens for subprocess use.
This provides a clean interface for runner-env.ts to get tokens without
caching, ensuring account changes are reflected immediately.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* auto-claude: subtask-1-3 - Update getRunnerEnv() to include GITHUB_TOKEN

* auto-claude: subtask-2-1 - Add auth change detection and event emission to oauth-handlers.ts

- Add GitHubAuthChangedPayload interface for auth change events
- Add sendAuthChangedToRenderer() to broadcast auth changes to all windows
- Add getCurrentGitHubUsername() helper to get current GitHub user
- Modify registerStartGhAuth() to:
  - Capture username before auth starts
  - Get username after successful auth
  - Emit GITHUB_AUTH_CHANGED event if account changed

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* auto-claude: subtask-3-1 - Add onGitHubAuthChanged listener to GitHubAPI interface

- Add onGitHubAuthChanged to GitHubAPI interface in github-api.ts
- Add implementation using createIpcListener with IPC_CHANNELS.GITHUB_AUTH_CHANGED
- Add mock implementation in browser-mock.ts for testing

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* auto-claude: subtask-4-1 - Add GitHub auth change listener to pr-review-store

* fix: Address PR review findings for GitHub auth handlers

- Convert getCurrentGitHubUsername() to async using promisified execFile
  to avoid blocking Electron main thread during auth flow (finding #1)
- Make getGitHubTokenForSubprocess() truly async by introducing async
  getTokenFromGhCliAsync() - the sync version is preserved for
  getGitHubConfig (finding #2)
- Add warning log when username fetch fails after successful auth,
  handling the edge case where auth succeeds but account change
  detection fails (finding AndyMik90#3)
- Remove unused timestamp field from GitHubAuthChangedPayload interface
  since the renderer callback only uses oldUsername/newUsername (finding AndyMik90#4)
- Document the intentional extraEnv override behavior in getRunnerEnv()
  JSDoc comment (finding AndyMik90#5)

All 5 findings from PR review were real issues. These fixes improve code
quality by avoiding main thread blocking and clarifying edge cases.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* test: Fix oauth-handlers tests for async getCurrentGitHubUsername

Update test mocks and add waitForAsyncSetup helper to handle the async
changes in getCurrentGitHubUsername(). The function now uses promisified
execFile instead of execFileSync to avoid blocking the main thread.

Key changes:
- Add mockExecFile mock for the promisified execFile function
- Add waitForAsyncSetup helper to wait for async setup before emitting
  process events
- Update all affected tests to use waitForAsyncSetup before emitting
  mock process events

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
kaigler added a commit that referenced this pull request Jan 30, 2026
…lse positives

- Fix dual status emission in worktree handlers (#1, HIGH): route
  merge/discard status changes through TaskStateManager instead of
  direct IPC emission. Add human_review case to handleManualStatusChange.
- Extract duplicate phaseMap to shared XSTATE_TO_PHASE constant (AndyMik90#6, LOW)
- Add --force flag in spec_runner.py when chaining to run.py after
  auto-approved specs to prevent BUILD BLOCKED hash mismatch errors
- Guard duplicate CODING_STARTED emission in coder.py (AndyMik90#8, MEDIUM):
  skip second emit when just_transitioned_from_planning is True
- Simplify stuck detection to 60s catastrophic-only check: XState
  handles all normal process-exit transitions via PROCESS_EXITED events.
  Remove phase-skip logic, visibility handler, and 5s/30s timers.
- Record task activity on status changes and log events (not just
  execution progress) to prevent false positive stuck detection
- Add tests for activity recording and human_review manual status change

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
kaigler added a commit that referenced this pull request Jan 31, 2026
…yMik90#1338) (AndyMik90#1575)

* feat: add backend task event protocol

* fix: harden spec_runner project detection

* feat: parse task events and track sequences

* feat: add xstate task machine

* feat: wire task events into state manager

* refactor: centralize status handling in state manager

* feat: hydrate task state and propagate reviewReason

* auto-claude: subtask-1-1 - Create card_data.txt file with literal string 'card data'

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

* fix: skip stuck detection for QA phases to prevent race conditions

Added qa_review and qa_fixing to the stuck detection skip list in both
TaskCard.tsx and useTaskDetail.ts. When the process exits unexpectedly
during QA phases, XState handles transitioning to error state. Skipping
stuck detection for these phases avoids race conditions where the stuck
check fires before the status update IPC reaches the renderer.

Also added unit tests for task-machine (35 tests) and task-state-manager
(20 tests), plus XSTATE_MIGRATION_SUMMARY.md documenting the migration.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: use XState as source of truth instead of stale cache

- Add getCurrentState() and isInPlanReview() methods to TaskStateManager
- Fix TASK_START handler to check XState actor state before falling back to task data
- Fix handleManualStatusChange to use XState state for determining correct event
- Prevents wrong event being sent when plan approval happens with stale cached data
- Add debug logging throughout state transitions for troubleshooting

The root cause was that when approving a plan, the UI called startTask() which
used cached task data (3-second TTL) to determine which XState event to send.
If the cache was stale, it would send USER_RESUMED instead of PLAN_APPROVED,
causing the task to transition incorrectly.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: prevent plan updates from overwriting XState-controlled status

When TASK_PROGRESS events arrived with stale plan data containing
status: 'in_progress', updateTaskFromPlan was overwriting the correct
XState-set status (e.g., 'ai_review'), causing tasks to jump back
to the wrong Kanban column.

XState is now the sole source of truth for task status. Plan updates
only update subtasks, title, and other non-status fields. Status changes
only come through TASK_STATUS_CHANGE events emitted by XState.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: remove AndyMik90#1585 code from PR

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: ruff lint - remove unnecessary string annotation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: biome lint and ruff format fixes

- Wrap case 'in_progress' block with braces in task-state-manager.ts
- Apply ruff format to 5 Python files

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: resolve flaky subprocess-spawn test on Windows CI

The 'should track running tasks' test was failing intermittently on
Windows CI because both tasks share the same mockProcess, and the
timing of exit event handlers could vary between environments.

Changes:
- Emit exit events twice to ensure both handlers receive them
- Use Promise.allSettled to wait for both tasks
- Add 100ms delay for event handlers to complete on slower CI

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: address PR AndyMik90#1575 review findings and stuck detection false positives

- Fix dual status emission in worktree handlers (#1, HIGH): route
  merge/discard status changes through TaskStateManager instead of
  direct IPC emission. Add human_review case to handleManualStatusChange.
- Extract duplicate phaseMap to shared XSTATE_TO_PHASE constant (AndyMik90#6, LOW)
- Add --force flag in spec_runner.py when chaining to run.py after
  auto-approved specs to prevent BUILD BLOCKED hash mismatch errors
- Guard duplicate CODING_STARTED emission in coder.py (AndyMik90#8, MEDIUM):
  skip second emit when just_transitioned_from_planning is True
- Simplify stuck detection to 60s catastrophic-only check: XState
  handles all normal process-exit transitions via PROCESS_EXITED events.
  Remove phase-skip logic, visibility handler, and 5s/30s timers.
- Record task activity on status changes and log events (not just
  execution progress) to prevent false positive stuck detection
- Add tests for activity recording and human_review manual status change

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix(lint): ruff format spec_runner.py long lines

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix(test): resolve flaky subprocess-spawn test on Windows CI

Wait for spawn promises to fully resolve before emitting exit events,
ensuring exit handlers are attached. A single setImmediate was insufficient
on Windows CI where async operations (getAPIProfileEnv, getRecoveryCoordinator)
between addProcess and .on('exit') take longer.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: address PR review findings for XState refactor

- CMT-001 [HIGH]: Add 'queue' and 'queued' status mappings to statusMap
  in project-store.ts to prevent task regression from queue to backlog
  when loading from disk
- NEW-003 [MEDIUM]: Integrate clearAllTasks() into TASK_LIST handler's
  forceRefresh path and update documentation to reflect actual usage
- CMT-003 [MEDIUM]: Change fail-open to fail-closed pattern in
  spec_runner.py - default require_review=True when JSON parsing fails

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: address security and quality findings from PR review

Security fixes:
- NEW-006 [HIGH]: Add path traversal protection in TASK_CREATE and
  TASK_UPDATE image handlers using path.basename() sanitization and
  resolved path validation
- NEW-005 [MEDIUM]: Add MIME type validation against allowlist in
  TASK_CREATE and TASK_UPDATE, consistent with TASK_REVIEW

Quality fixes:
- NEW-004 [LOW]: Add debug logging when context not found during
  XState state transitions to aid debugging
- NEW-REVIEW-003 [MEDIUM]: Preserve lastSequenceByTask during
  clearAllTasks() to prevent duplicate event processing if backend
  events arrive during refresh window

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* test: update clearAllTasks test to expect preserved sequence tracking

The test was expecting sequences to be cleared after clearAllTasks(),
but the implementation was changed to preserve lastSequenceByTask to
prevent duplicate event processing during the refresh window.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
Co-authored-by: AndyMik90 <andre@mikalsenutvikling.no>
Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/fullstack bug Something isn't working size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant